feat(KONFLUX-4136): add new reduce step #1969
Conversation
scoheb
commented
Sep 17, 2024
scoheb
commented
- add new step to reduce snapshot for single component mode
- requires feat(KONFLUX-4136): new reduce-snapshot script #1968
scoheb
force-pushed
the
KONFLUX-4136-update-task
branch
from
f666b12 to
dcc6954
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1969 +/- ##
=======================================
Coverage 74.11% 74.11%
=======================================
Files 88 88
Lines 5729 5729
=======================================
Hits 4246 4246
Misses 1483 1483
Flags with carried forward coverage won't be shown. Click here to find out more. |
| @@ -177,7 +212,7 @@ spec: | |||
| - image | |||
| - "--verbose" | |||
| - "--images" | |||
| - "$(params.IMAGES)" | |||
| - "$(params.HOMEDIR)/snapshot.json" | |||
There was a problem hiding this comment.
Should this be $(params.SNAPSHOT_PATH)?
Also, not sure if we want to expose this as a parameter to the user. It seems more like an implementation detail.
There was a problem hiding this comment.
for Integration Test Scenarios, SNAPSHOT is the json representation of the snapshot.
for Release Pipelines, SNAPSHOT will be the path to the json file.
With my latest PR #1972 , I am always outputting a JSON file that will either contain the complete Snapshot or the reduced one.
so yes, once that PR merges, I can use $(params.SNAPSHOT_PATH).
It seems more like an implementation detail
Are you meaning to say it should not a param, and I am simply hard-code /tekton/home/snapshot.json?
There was a problem hiding this comment.
Are you meaning to say it should not a param, and I am simply hard-code /tekton/home/snapshot.json?
Yeah, exactly.
There was a problem hiding this comment.
It's minor, but I'd rather reduce the amount of parameters introduced. We can always add it later if there's a need. That's easier than removing a parameter.
There was a problem hiding this comment.
removed parameter and using the already defined HOME parameter in the arg
| type: string | ||
| default: "false" | ||
|
|
||
| - name: CUSTOM_RESOURCE |
There was a problem hiding this comment.
The name and description of this parameter, and the one below, should reflect that is associated with SINGLE_COMPONENT mode. Consider:
- name: SINGLE_COMPONENT_NAME
description: >
Name, including kind, of the Kubernetes resource to query for labels when single
component mode is enabled, e.g. pr/somepipeline.
- name: SINGLE_COMPONENT_NAMESPACE
description: >
Kubernetes namespace where the SINGLE_COMPONENT_NAME is found. Only used
when single component mode is enabled.
There was a problem hiding this comment.
Maybe SINGLE_COMPONENT_CR and SINGLE_COMPONENT_NS.
There was a problem hiding this comment.
Is it ever not a pipeline run? Maybe SINGLE_COMPONENT_PR ?
There was a problem hiding this comment.
Since you need to specify the kind, the implication is it might be something other than a PipelineRun. Is that the intention?
There was a problem hiding this comment.
If we have to change it, then I would prefer SINGLE_COMPONENT_CUSTOM_RESOURCE and SINGLE_COMPONENT_CUSTOM_RESOURCE_NAMESPACE
Is it ever not a pipeline run? Maybe SINGLE_COMPONENT_PR ?
Yes in Release Pipelines, it is a Snapshot
There was a problem hiding this comment.
I would prefer
SINGLE_COMPONENT_CUSTOM_RESOURCEandSINGLE_COMPONENT_CUSTOM_RESOURCE_NAMESPACE
Works for me.
There was a problem hiding this comment.
It's quite long I guess. SINGLE_COMPONENT_CUSTOM_RESOURCE_NS maybe?
There was a problem hiding this comment.
I am ok with SINGLE_COMPONENT_CUSTOM_RESOURCE and SINGLE_COMPONENT_CUSTOM_RESOURCE_NS
|
I think the acceptance test failure is legit: |
yes. hence #1972 |
scoheb
force-pushed
the
KONFLUX-4136-update-task
branch
from
8757e8d to
2fa7e16
Compare
|
/ok-to-test |
|
Let's just get the commits organized to better represent the change before merging. |
- add new step to reduce snapshot for single component mode Signed-off-by: Scott Hebert <[email protected]>
scoheb
force-pushed
the
KONFLUX-4136-update-task
branch
from
2258b53 to
908b47d
Compare
